CIL: Account for multiple VariableUpdate::getSource()s in nullness analysis#1368
Conversation
…analysis For methods compiled without optimization (and possibly also with optimization), it is possible for a variable update to have multiple possible assigned values. For example, the non-optimized CIL for ``` return cond ? null : "not null" ``` is ``` 0: nop 1: ldarg.0 2: ldfld cond 3: brtrue.s 6: 4: ldstr "not null" 5: br.s 7: 6: ldnull 7: stloc.0 L0 // stores either `null` or "not null" 8: br.s 9: 9: ldloc.0 10: ret ``` Consequently, an existential in `CallableReturns.qll` must be a `forex`.
calumgrant
left a comment
calumgrant
left a comment
There was a problem hiding this comment.
This is really great stuff, and I like that you have taken care to produce binaries to reproduce the result. I have one general question, and happy to merge as-is.
| or | ||
| forex(VariableUpdate vu | DefUse::variableUpdateUse(_, vu, expr) | | ||
| alwaysNotNullExpr(vu.getSource()) | ||
| forex(Expr src | src = vu.getSource() | alwaysNotNullExpr(src)) |
There was a problem hiding this comment.
Great sleuthing. Do you think it makes sense to rename getSource to getASource? The problem is that quite a few methods could be renamed to getA... for expressions, due to the multi-valued nature of optimized code?
There was a problem hiding this comment.
I did consider it, but I don't think it is worthwhile. Actually, it is getOperand(int i) that can have multiple values, so I wonder if we could end up in a situation where an instruction that pops two values could get mixed operands:
1. ldfld cond
2. brtrue.s 6:
3. ldstr "a"
4: ldstr "b"
5: br.s 8:
6: ldstr "c"
7: ldstr "d"
8. // instruction that pops two values
I.e., I wonder if we could end up with the instruction 7 having e.g. getOperand(0) = "a" and getOperand(1) = "d".
|
Here is the performance report: https://git.semmle.com/gist/tom/2cdcfed314fd154cc98ac7ff35566996 (internal link), which shows that performance is unchanged. |
2 participants
For methods compiled without optimization (and possibly also with optimization), it is possible for a variable update to have multiple possible assigned values. For example, the non-optimized CIL for
is
Consequently, an existential in
CallableReturns.qllmust be aforex.This change eliminates all the false positives here: https://lgtm.com/projects/g/sharpsuite/sharpinit/alerts/?mode=tree&ruleFocus=1506094316834